-
Notifications
You must be signed in to change notification settings - Fork 8.1k
STM32: Replace HAL register operations #97329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
STM32: Replace HAL register operations #97329
Conversation
|
Waiting for test results to unblock. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if all the casts the mem_addr_t could be hidden some way with helper macros or inline functions. Outside of the scope of this P-R.
7b6ab32 to
c3ab6e2
Compare
3658a6a to
f97903d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few review comments that may be addressed later but an issue to fix in drivers/adc/adc_stm32.c set_reg_value().
a5f466b to
ea37e79
Compare
|
Rebased to fix conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LAGTM
drivers/adc/adc_stm32.c
Outdated
|
|
||
| uintptr_t addr = (uintptr_t)adc + reg; | ||
| size_t reg32_offset = reg / sizeof(uint32_t); | ||
| uint32_t *addr = (uint32_t *)config->base + reg32_offset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| uint32_t *addr = (uint32_t *)config->base + reg32_offset; | |
| volatile uint32_t *addr = (volatile uint32_t *)config->base + reg32_offset; |
This should have raised a compiler warning (I didn't check if it did or not)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+ outside PR scope but now that the STM32 helper functions exist, maybe we don't need these {get,set}_reg_value() helpers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have raised a compiler warning (I didn't check if it did or not)
The many places where stm32_reg_xxx() are used use a pointer argument that is not an explicit volatile data pointer. I don't think it's needed here. Actaully, I don't think the address argument of these helper functions need to be volatile. The cast is done in the sys_xxx() functions and that should be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have raised a compiler warning (I didn't check if it did or not)
The many places where
stm32_reg_xxx()are used use a pointer argument that is not an explicitvolatiledata pointer. I don't think it's needed here. Actaully, I don't think theaddressargument of these helper functions need to bevolatile. The cast is done in the sys_xxx() functions and that should be enough.
You're right, it is probably allowed to up-cast a uint32_t * to volatile uint32_t * because the former is less strict than the latter (same as const uint32_t -> const uint32_t *). Still better to cast to proper type explicitly IMO.
Add a set of bitops functions in order to replace the STM32 HAL bitops macros throughout the drivers. Signed-off-by: Guillaume Gautier <[email protected]>
For all STM32 drivers, replace the SET_BIT macro (defined in the STM32 HAL) by stm32_reg_set_bits defined in Zephyr. Signed-off-by: Guillaume Gautier <[email protected]>
For all STM32 drivers, replace the CLEAR_BIT macro (defined in the STM32 HAL) by stm32_reg_clear_bits defined in Zephyr. Signed-off-by: Guillaume Gautier <[email protected]>
For all STM32 drivers and SoC, replace the READ_BIT macro (defined in the STM32 HAL) by stm32_reg_read_bits. Fixes some cases where the return value was tested like a boolean despite not being one. Signed-off-by: Guillaume Gautier <[email protected]>
For all STM32 drivers and SoC, replace the WRITE_REG macro and the LL_xxx_WriteReg functions (defined in the STM32 HAL) by stm32_reg_write defined in Zephyr. Signed-off-by: Guillaume Gautier <[email protected]>
For all STM32 drivers and SoC, replace the READ_REG macro and the LL_xxx_ReadReg functions (defined in the STM32 HAL) by stm32_reg_read defined in Zephyr. Signed-off-by: Guillaume Gautier <[email protected]>
For all STM32 drivers and SoC, replace the MODIFY_REG macro (defined in the STM32 HAL) by stm32_reg_modify_bits defined in Zephyr. Signed-off-by: Guillaume Gautier <[email protected]>
Call stm32_reg_set_bits instead of stm32_reg_read then stm32_reg_write. Signed-off-by: Guillaume Gautier <[email protected]>
Use dedicated set/clear bits function instead of reading/masking/writing manually. Signed-off-by: Guillaume Gautier <[email protected]>
ea37e79 to
b18ff3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM.
|



This PR replaces the STM32 HAL register operations macros (such as WRITE_REG or MODIFY_REG) and functions (such as LL_I2C_WriteReg) by Zephyr equivalents in all STM32 drivers and SoC.